Fixed Allocation Explain API in batch mode #10348
Conversation
Signed-off-by: Shivansh Arora <[email protected]>
Compatibility status:Checks if related components are compatible with change a730ff5 Incompatible componentsSkipped componentsCompatible components |
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Shivansh Arora <[email protected]>
There was a problem hiding this comment.
This file is originally added in #8746. I have added a new test below in this file. The diff will updated once that PR is merged and I rebase my changes on top of that.
| .flatMap(List::stream).collect(Collectors.toList()); | ||
| allShardRoutings.forEach(shard -> assertNull(testGatewayAllocator.getBatchId(shard, shard.primary()))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Only following test is added in this PR
shiv0408
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
peternied,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
VachaShah,
dbwiddis,
sachinpkale and
sohami
as code owners
Gradle Check (Jenkins) Run Completed with:
|
server/src/main/java/org/opensearch/gateway/GatewayAllocator.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Shivansh Arora <[email protected]>
shiv0408
requested review from
abbashus,
adnapibar and
tlfeng
as code owners
|
❌ Gradle check result for a730ff5: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| @@ -113,6 +113,17 @@ protected GatewayAllocator() { | |||
| this.replicaShardAllocator = null; | |||
| } | |||
|
|
|||
| // for tests | |||
| protected void setShardAllocators(PrimaryShardAllocator primaryShardAllocator, | |||
There was a problem hiding this comment.
There is already a constructor that is exposed for tests.
We expose protected methods for testing a method behaviour per say. For example we have exposed protected static void innerAllocatedUnassigned that takes allocators as argument.
The way you are exposing makes the room potential data loss or state loss for GatewayAllocator state, since GatewayAllocator is a singleton object for that reason itself in the memory. I would highly appreaciate to do away with this function as it can cause a potential state loss leak in GatewayAllocator
There was a problem hiding this comment.
I did tried writing this test without above code, but could not found any way to do it. If you have any other approach how we test the usecase without using this, happy to try that out.
| assertEquals(0, testGatewayAllocator.getNumberOfStoreShardBatches()); | ||
|
|
||
| // now calling allocation explain to ensure that batches are getting created | ||
| testAllocation.debugDecision(true); |
There was a problem hiding this comment.
Also assert API response is getting returned
|
|
||
| @Override | ||
| public AllocateUnassignedDecision explainUnassignedShardAllocation(ShardRouting unassignedShard, RoutingAllocation routingAllocation) { | ||
| setShardAllocators(primaryShardAllocator, replicaShardAllocator, primaryBatchShardAllocator, replicaBatchShardAllocator); |
There was a problem hiding this comment.
I reference to the above comment on setAllocators, trying add implementing a new GatewayAllocator like this in this test.
https://github.com/Gaurav614/OpenSearch/blob/collab-central/server/src/test/java/org/opensearch/cluster/routing/allocation/AllocationServiceTests.java#L263
I am also open for more suggestions /discussions/ challenges you might have implementing this
| private PrimaryShardAllocator primaryShardAllocator; | ||
| private ReplicaShardAllocator replicaShardAllocator; |
There was a problem hiding this comment.
As discussed, let us add back the final modifier and instead of a UT, we can use an Integration test for explainUnassigned in batch mode scenario.
|
This PR is stalled because it has been open for 30 days with no activity. |
|
Hi @shiv0408, Is this being worked upon? Feel free to reach out to maintainers for further reviews. |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
Discarding this PR as the core logic of explainUnassignedShardAllocation written in this PR is already added in #8746. Will add the test added in this PR to the above mentioned PR only. |
Description
This Pull request fixes the
explainUnassignedShardAllocationmethod to use the correct batch allocator to find unassigned explain reason in case the batch mode is enabled.Related Issues
This is part of improvement for #5098
Related to #8098
Dependency
This change is dependent on #8865, which needs to be merged before merging this PR
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.